Skip to content

RUST-1529 Use AWS SDK for sigv4 signing #1438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

JamieTsai1024
Copy link
Contributor

@JamieTsai1024 JamieTsai1024 commented Jul 30, 2025

Replace existing implementation for getting sigv4 signing using the AWS SDK.

Testing

Clone https://github.com/isabelatkinson/drivers-evergreen-tools/tree/local-aws and run the following commands:

./.evergreen/run-aws-auth.sh

cd .evergreen/auth_aws
# run this script to reconfigure the type of AWS authentication to use
# copy the block of unset and export commands output by the script and run them in the terminal window you're using to run the driver tests
./aws_setup.sh (regular | env-creds | assume-role | session-creds | web-identity)

cd ~/mongo-rust-driver 
# Run the unset and set export commands printed by `./aws_setup.sh` for environment variables 
cargo nextest run auth_aws --features aws-auth

Previously

  • Used AWS SDK to retrieve credentials in PR #1435

Work to be done for RUST-1529

  • Decide whether to keep AWS SDK behind feature flag or replace original implementation

JamieTsai1024 and others added 30 commits July 23, 2025 12:30
@@ -117,27 +126,34 @@ async fn authenticate_stream_inner(
let creds = get_aws_credentials(credential).await.map_err(|e| {
Error::authentication_error(MECH_NAME, &format!("failed to get creds: {e}"))
})?;
let aws_credential = AwsCredential::from_sdk_creds(creds);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a previous PR, we introduced from_sdk_creds(...) to convert from aws_credential_types::Credentials to locally defined AwsCredential type. Now that we've converted the rest of the authentication to use the AWS SDK, we no longer need this conversion

@JamieTsai1024 JamieTsai1024 marked this pull request as ready for review August 1, 2025 18:06
@JamieTsai1024 JamieTsai1024 requested a review from a team as a code owner August 1, 2025 18:06
@JamieTsai1024 JamieTsai1024 requested a review from abr-egn August 1, 2025 18:06
@@ -223,7 +223,7 @@ buildvariants:

- name: aws-auth
display_name: "AWS Authentication"
patchable: false
# patchable: false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will uncomment once PR is approved

request.headers().iter().map(|(k, v)| {
(
k.as_str(),
std::str::from_utf8(v.as_bytes()).expect("Header value should be valid UTF-8"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will panic if the conversion fails; instead, we should propagate an authentication error

Comment on lines +287 to +289
let (signing_instructions, _signature) = sign(signable_request, &signing_params)
.map_err(|e| Error::authentication_error(MECH_NAME, &format!("Signing failed: {e}")))?
.into_parts();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since we're not using the returned signature, I think this can call output rather than into_parts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Isabel! I tried switching to .output(), but ran into a borrow checker error when calling apply_to_request_http1x on line 291. I double-checked the method signature and saw that it needs an owned value which we get with .into_parts(), whereas .output() returns a reference.

The .into_parts() also matches what I saw in the two references we looked at (1, 2). Let me know if you had a different approach in mind!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I missed the reference vs. owned value distinction, feel free to leave as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, thanks!

Comment on lines +287 to +289
let (signing_instructions, _signature) = sign(signable_request, &signing_params)
.map_err(|e| Error::authentication_error(MECH_NAME, &format!("Signing failed: {e}")))?
.into_parts();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I missed the reference vs. owned value distinction, feel free to leave as-is

})?;
Ok((k.as_str(), value))
})
.filter_map(Result::ok),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will ignore any failures that occur when converting the strings and proceed with authentication. This would likely cause the signature to be computed incorrectly, and the reported error might be confusing for users to parse. Instead, I suggest doing the following to propagate an error here:

let headers: Result<Vec<_>> = request
    .headers()
    .iter()
    .map(|(k, v)| {
        let v = v.to_str().map_err(|_| {
            Error::authentication_error(
                MECH_NAME,
                "Failed to convert header value to valid UTF-8",
            )
        })?;
        Ok((k.as_str(), v))
    })
    .collect();

This uses a handy iterator trick: if an iterator is mapped into Result values, you can collect it into a Result<Vec<_>> (or a Result of any collection type). That result will either contain Ok(Vec<_>) if all of the values were Ok, or an Err(_) containing the first error that was encountered.

We then need to return an error if needed using ? and turn the value back into an iterator, which is the type accepted by SignableRequest::new:

headers?.into_iter()

This should make what happened more clear if something goes wrong with the headers.

@isabelatkinson
Copy link
Contributor

The rustdoc error is unrelated, you can merge with main to pull in a fix for it

isabelatkinson
isabelatkinson previously approved these changes Aug 8, 2025
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @abr-egn this is ready for your review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants